-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
transport: implement graphql-transport-ws
ws sub-protocol
#1507
transport: implement graphql-transport-ws
ws sub-protocol
#1507
Conversation
It looks like the integration tests failed because of an issue with the docker registry.
Is there a way to run the tests again? |
Thanks for the PR - ran tests again for you. |
Would be great to have this! |
Starting from your PR, I've extended it and adding duplex ping pong option here #1578 |
this would be awesome |
Hello, any chance this will get merged, please? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@StevenACoffman asked me to take a look at this, and it makes sense to me! I don't have much experience with graphql subscriptions though, so take it with a grain of salt. It would be nice to have an integration test that actually fired up a server and verified that the nodejs graphql-ws
library can talk to it, but the included example is probably the next best thing.
@@ -86,17 +86,21 @@ func (t Websocket) Do(w http.ResponseWriter, r *http.Request, exec graphql.Graph | |||
} | |||
|
|||
func (c *wsConnection) init() bool { | |||
message := c.readOp() | |||
if message == nil { | |||
m, err := c.me.NextMessage() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love single-character variable names
c.mu.Lock() | ||
c.conn.WriteJSON(msg) | ||
// TODO: missing error handling here, err from previous implementation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you make a ticket for this error handling, and then do TODO(#1234)
with the issue number?
@@ -152,26 +158,27 @@ func (c *wsConnection) run() { | |||
|
|||
for { | |||
start := graphql.Now() | |||
message := c.readOp() | |||
if message == nil { | |||
m, err := c.me.NextMessage() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: variable naming
var text string | ||
switch t { | ||
default: | ||
text = "unknown" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: imo these would read better as return
s
handler.SendNextSubscriptionMessage() | ||
msg = readOp(c) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you add a comment here about why we expect the subscription message to be received twice?
@jordanabderrachid if you can address the review from jared above and resolve the conflicts, we can get this merged! Thanks! |
@jordanabderrachid are you still there? |
@frederikhors It's been long enough that if you (or anyone) want to fork from the good work of @jordanabderrachid and make a new PR, he'll still get his credit for his original commits. |
Co-authored-by: Jared Forsyth <jabapyth@gmail.com> Signed-off-by: Steve Coffman <steve@khanacademy.org>
c322e11
to
07191a6
Compare
@zdraganov I rebased but I would appreciate it if in your PR you could take care of adding a comment as mentioned above describing why we expect the subscription message to be received twice. Thanks! |
This implements the
graphql-transport-ws
websocket sub-protocol.I have refactored the sub-protocol message parsing by creating a generic message
message
and amessageExchanger
interface defined in the file websocket_subprotocol.go.The previously supported
graphql-ws
sub-protocol is implemented in the file websocket_graphqlws.go. The new protocol is implemented in the websocket_graphql_transport_ws.go.I've aimed to keep this change as much backward compatible as possible, the default sub-protocol selected is
graphql-ws
, even if the ws client does not explicitly request it. I am also not requiring the consumer of theTransport
struct to provide the list of supported sub-protocols, and they are injected at runtime instead.In addition to that, I have fixed some libraries version mismatch in the
example/chat
example project. I have included another implementation of the websocket link that uses the newly supported sub-protocol.Closes #1430
I have: